[VPlan] Move recording of Inst->VPValue to VPRecipeBuilder (NFCI). #84464

fhahn · 2024-03-08T11:24:44Z

Instead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands).

By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts.

Instead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands). By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts.

llvmbot · 2024-03-08T11:25:18Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Instead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands).

By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts.

Patch is 21.87 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84464.diff

8 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+29-40)
(modified) llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h (+16-17)
(modified) llvm/lib/Transforms/Vectorize/VPlan.cpp (+5-5)
(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+6-27)
(modified) llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp (+2-2)
(modified) llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp (+4-5)
(modified) llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp (+1-1)
(modified) llvm/unittests/Transforms/Vectorize/VPlanTest.cpp (+2-2)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index edaad4d033bdf0..f52843c7e2d8c3 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -7913,6 +7913,18 @@ void LoopVectorizationPlanner::buildVPlans(ElementCount MinVF,
   }
 }
 
+iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
+VPRecipeBuilder::mapToVPValues(User::op_range Operands, VPlan &Plan) {
+  std::function<VPValue *(Value *)> Fn = [this, &Plan](Value *Op) {
+    if (auto *I = dyn_cast<Instruction>(Op)) {
+      if (auto *R = Ingredient2Recipe.lookup(I))
+        return R->getVPSingleValue();
+    }
+    return Plan.getOrAddLiveIn(Op);
+  };
+  return map_range(Operands, Fn);
+}
+
 VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
                                          VPlan &Plan) {
   assert(is_contained(predecessors(Dst), Src) && "Invalid edge");
@@ -7938,7 +7950,7 @@ VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
   if (OrigLoop->isLoopExiting(Src))
     return EdgeMaskCache[Edge] = SrcMask;
 
-  VPValue *EdgeMask = Plan.getVPValueOrAddLiveIn(BI->getCondition());
+  VPValue *EdgeMask = getVPValue(BI->getCondition(), Plan);
   assert(EdgeMask && "No Edge Mask found for condition");
 
   if (BI->getSuccessor(0) != Dst)
@@ -7949,7 +7961,7 @@ VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
     // 'select i1 SrcMask, i1 EdgeMask, i1 false'.
     // The select version does not introduce new UB if SrcMask is false and
     // EdgeMask is poison. Using 'and' here introduces undefined behavior.
-    VPValue *False = Plan.getVPValueOrAddLiveIn(
+    VPValue *False = Plan.getOrAddLiveIn(
         ConstantInt::getFalse(BI->getCondition()->getType()));
     EdgeMask =
         Builder.createSelect(SrcMask, EdgeMask, False, BI->getDebugLoc());
@@ -8151,7 +8163,7 @@ VPWidenIntOrFpInductionRecipe *VPRecipeBuilder::tryToOptimizeInductionTruncate(
 
     auto *Phi = cast<PHINode>(I->getOperand(0));
     const InductionDescriptor &II = *Legal->getIntOrFpInductionDescriptor(Phi);
-    VPValue *Start = Plan.getVPValueOrAddLiveIn(II.getStartValue());
+    VPValue *Start = Plan.getOrAddLiveIn(II.getStartValue());
     return createWidenInductionRecipes(Phi, I, Start, II, Plan, *PSE.getSE(),
                                        *OrigLoop, Range);
   }
@@ -8263,7 +8275,7 @@ VPWidenCallRecipe *VPRecipeBuilder::tryToWidenCall(CallInst *CI,
       if (Legal->isMaskRequired(CI))
         Mask = getBlockInMask(CI->getParent());
       else
-        Mask = Plan->getVPValueOrAddLiveIn(ConstantInt::getTrue(
+        Mask = Plan->getOrAddLiveIn(ConstantInt::getTrue(
             IntegerType::getInt1Ty(Variant->getFunctionType()->getContext())));
 
       Ops.insert(Ops.begin() + *MaskPos, Mask);
@@ -8306,8 +8318,8 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(Instruction *I,
     if (CM.isPredicatedInst(I)) {
       SmallVector<VPValue *> Ops(Operands.begin(), Operands.end());
       VPValue *Mask = getBlockInMask(I->getParent());
-      VPValue *One = Plan->getVPValueOrAddLiveIn(
-          ConstantInt::get(I->getType(), 1u, false));
+      VPValue *One =
+          Plan->getOrAddLiveIn(ConstantInt::get(I->getType(), 1u, false));
       auto *SafeRHS =
          new VPInstruction(Instruction::Select, {Mask, Ops[1], One},
                            I->getDebugLoc());
@@ -8402,7 +8414,7 @@ VPReplicateRecipe *VPRecipeBuilder::handleReplication(Instruction *I,
     BlockInMask = getBlockInMask(I->getParent());
   }
 
-  auto *Recipe = new VPReplicateRecipe(I, Plan.mapToVPValues(I->operands()),
+  auto *Recipe = new VPReplicateRecipe(I, mapToVPValues(I->operands(), Plan),
                                        IsUniform, BlockInMask);
   return Recipe;
 }
@@ -8417,10 +8429,6 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(
     if (Phi->getParent() != OrigLoop->getHeader())
       return tryToBlend(Phi, Operands, Plan);
 
-    // Always record recipes for header phis. Later first-order recurrence phis
-    // can have earlier phis as incoming values.
-    recordRecipeOf(Phi);
-
     if ((Recipe = tryToOptimizeInductionPHI(Phi, Operands, *Plan, Range)))
       return Recipe;
 
@@ -8445,14 +8453,6 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(
       PhiRecipe = new VPFirstOrderRecurrencePHIRecipe(Phi, *StartV);
     }
 
-    // Record the incoming value from the backedge, so we can add the incoming
-    // value from the backedge after all recipes have been created.
-    auto *Inc = cast<Instruction>(
-        Phi->getIncomingValueForBlock(OrigLoop->getLoopLatch()));
-    auto RecipeIter = Ingredient2Recipe.find(Inc);
-    if (RecipeIter == Ingredient2Recipe.end())
-      recordRecipeOf(Inc);
-
     PhisToFix.push_back(PhiRecipe);
     return PhiRecipe;
   }
@@ -8518,7 +8518,7 @@ void LoopVectorizationPlanner::buildVPlansWithVPRecipes(ElementCount MinVF,
 static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
                                   DebugLoc DL) {
   Value *StartIdx = ConstantInt::get(IdxTy, 0);
-  auto *StartV = Plan.getVPValueOrAddLiveIn(StartIdx);
+  auto *StartV = Plan.getOrAddLiveIn(StartIdx);
 
   // Add a VPCanonicalIVPHIRecipe starting at 0 to the header.
   auto *CanonicalIVPHI = new VPCanonicalIVPHIRecipe(StartV, DL);
@@ -8541,7 +8541,7 @@ static void addCanonicalIVRecipes(VPlan &Plan, Type *IdxTy, bool HasNUW,
 // Add exit values to \p Plan. VPLiveOuts are added for each LCSSA phi in the
 // original exit block.
 static void addUsersInExitBlock(VPBasicBlock *HeaderVPBB, Loop *OrigLoop,
-                                VPlan &Plan) {
+                                VPRecipeBuilder &Builder, VPlan &Plan) {
   BasicBlock *ExitBB = OrigLoop->getUniqueExitBlock();
   BasicBlock *ExitingBB = OrigLoop->getExitingBlock();
   // Only handle single-exit loops with unique exit blocks for now.
@@ -8552,7 +8552,7 @@ static void addUsersInExitBlock(VPBasicBlock *HeaderVPBB, Loop *OrigLoop,
   for (PHINode &ExitPhi : ExitBB->phis()) {
     Value *IncomingValue =
         ExitPhi.getIncomingValueForBlock(ExitingBB);
-    VPValue *V = Plan.getVPValueOrAddLiveIn(IncomingValue);
+    VPValue *V = Builder.getVPValue(IncomingValue, Plan);
     Plan.addLiveOut(&ExitPhi, V);
   }
 }
@@ -8588,9 +8588,6 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
     if (!getDecisionAndClampRange(applyIG, Range))
       continue;
     InterleaveGroups.insert(IG);
-    for (unsigned i = 0; i < IG->getFactor(); i++)
-      if (Instruction *Member = IG->getMember(i))
-        RecipeBuilder.recordRecipeOf(Member);
   };
 
   // ---------------------------------------------------------------------------
@@ -8659,10 +8656,10 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
       SmallVector<VPValue *, 4> Operands;
       auto *Phi = dyn_cast<PHINode>(Instr);
       if (Phi && Phi->getParent() == HeaderBB) {
-        Operands.push_back(Plan->getVPValueOrAddLiveIn(
+        Operands.push_back(Plan->getOrAddLiveIn(
             Phi->getIncomingValueForBlock(OrigLoop->getLoopPreheader())));
       } else {
-        auto OpRange = Plan->mapToVPValues(Instr->operands());
+        auto OpRange = RecipeBuilder.mapToVPValues(Instr->operands(), *Plan);
         Operands = {OpRange.begin(), OpRange.end()};
       }
 
@@ -8677,10 +8674,6 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
           Instr, Operands, Range, VPBB, Plan);
       if (!Recipe)
         Recipe = RecipeBuilder.handleReplication(Instr, Range, *Plan);
-      for (auto *Def : Recipe->definedValues()) {
-        auto *UV = Def->getUnderlyingValue();
-        Plan->addVPValue(UV, Def);
-      }
 
       RecipeBuilder.setRecipe(Instr, Recipe);
       if (isa<VPHeaderPHIRecipe>(Recipe)) {
@@ -8712,7 +8705,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
     // and there is nothing to fix from vector loop; phis should have incoming
     // from scalar loop only.
   } else
-    addUsersInExitBlock(HeaderVPBB, OrigLoop, *Plan);
+    addUsersInExitBlock(HeaderVPBB, OrigLoop, RecipeBuilder, *Plan);
 
   assert(isa<VPRegionBlock>(Plan->getVectorLoopRegion()) &&
          !Plan->getVectorLoopRegion()->getEntryBasicBlock()->empty() &&
@@ -8774,16 +8767,12 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
       continue;
     Constant *CI = ConstantInt::get(Stride->getType(), ScevStride->getAPInt());
 
-    auto *ConstVPV = Plan->getVPValueOrAddLiveIn(CI);
+    auto *ConstVPV = Plan->getOrAddLiveIn(CI);
     // The versioned value may not be used in the loop directly, so just add a
     // new live-in in those cases.
-    Plan->getVPValueOrAddLiveIn(StrideV)->replaceAllUsesWith(ConstVPV);
+    Plan->getOrAddLiveIn(StrideV)->replaceAllUsesWith(ConstVPV);
   }
 
-  // From this point onwards, VPlan-to-VPlan transformations may change the plan
-  // in ways that accessing values using original IR values is incorrect.
-  Plan->disableValue2VPValue();
-
   VPlanTransforms::dropPoisonGeneratingRecipes(*Plan, [this](BasicBlock *BB) {
     return Legal->blockNeedsPredication(BB);
   });
@@ -10082,7 +10071,7 @@ bool LoopVectorizePass::processLoop(Loop *L) {
         EpilogILV.setTripCount(MainILV.getTripCount());
         for (auto &R : make_early_inc_range(*BestEpiPlan.getPreheader())) {
           auto *ExpandR = cast<VPExpandSCEVRecipe>(&R);
-          auto *ExpandedVal = BestEpiPlan.getVPValueOrAddLiveIn(
+          auto *ExpandedVal = BestEpiPlan.getOrAddLiveIn(
               ExpandedSCEVs.find(ExpandR->getSCEV())->second);
           ExpandR->replaceAllUsesWith(ExpandedVal);
           if (BestEpiPlan.getTripCount() == ExpandR)
@@ -10123,7 +10112,7 @@ bool LoopVectorizePass::processLoop(Loop *L) {
                 {EPI.MainLoopIterationCountCheck});
           }
           assert(ResumeV && "Must have a resume value");
-          VPValue *StartVal = BestEpiPlan.getVPValueOrAddLiveIn(ResumeV);
+          VPValue *StartVal = BestEpiPlan.getOrAddLiveIn(ResumeV);
           cast<VPHeaderPHIRecipe>(&R)->setStartValue(StartVal);
         }
 
diff --git a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
index b1498026adadfe..527417e29906fe 100644
--- a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
+++ b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
@@ -49,9 +49,8 @@ class VPRecipeBuilder {
   EdgeMaskCacheTy EdgeMaskCache;
   BlockMaskCacheTy BlockMaskCache;
 
-  // VPlan-VPlan transformations support: Hold a mapping from ingredients to
-  // their recipe. To save on memory, only do so for selected ingredients,
-  // marked by having a nullptr entry in this map.
+  // VPlan construction support: Hold a mapping from ingredients to
+  // their recipe.
   DenseMap<Instruction *, VPRecipeBase *> Ingredient2Recipe;
 
   /// Cross-iteration reduction & first-order recurrence phis for which we need
@@ -117,13 +116,8 @@ class VPRecipeBuilder {
                                        VFRange &Range, VPBasicBlock *VPBB,
                                        VPlanPtr &Plan);
 
-  /// Set the recipe created for given ingredient. This operation is a no-op for
-  /// ingredients that were not marked using a nullptr entry in the map.
+  /// Set the recipe created for given ingredient.
   void setRecipe(Instruction *I, VPRecipeBase *R) {
-    if (!Ingredient2Recipe.count(I))
-      return;
-    assert(Ingredient2Recipe[I] == nullptr &&
-           "Recipe already set for ingredient");
     Ingredient2Recipe[I] = R;
   }
 
@@ -146,14 +140,6 @@ class VPRecipeBuilder {
   /// between SRC and DST.
   VPValue *getEdgeMask(BasicBlock *Src, BasicBlock *Dst) const;
 
-  /// Mark given ingredient for recording its recipe once one is created for
-  /// it.
-  void recordRecipeOf(Instruction *I) {
-    assert((!Ingredient2Recipe.count(I) || Ingredient2Recipe[I] == nullptr) &&
-           "Recipe already set for ingredient");
-    Ingredient2Recipe[I] = nullptr;
-  }
-
   /// Return the recipe created for given ingredient.
   VPRecipeBase *getRecipe(Instruction *I) {
     assert(Ingredient2Recipe.count(I) &&
@@ -172,6 +158,19 @@ class VPRecipeBuilder {
   /// Add the incoming values from the backedge to reduction & first-order
   /// recurrence cross-iteration phis.
   void fixHeaderPhis();
+
+  /// Returns a range mapping the values the range \p Operands to their
+  /// corresponding VPValues.
+  iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
+  mapToVPValues(User::op_range Operands, VPlan &Plan);
+
+  VPValue *getVPValue(Value *V, VPlan &Plan) {
+    if (auto *I = dyn_cast<Instruction>(V)) {
+      if (auto *R = Ingredient2Recipe.lookup(I))
+        return R->getVPSingleValue();
+    }
+    return Plan.getOrAddLiveIn(V);
+  }
 };
 } // end namespace llvm
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 9768e4b7aa0a8a..53ee9793fdefe9 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -812,7 +812,7 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV,
   // needs to be changed from zero to the value after the main vector loop.
   // FIXME: Improve modeling for canonical IV start values in the epilogue loop.
   if (CanonicalIVStartValue) {
-    VPValue *VPV = getVPValueOrAddLiveIn(CanonicalIVStartValue);
+    VPValue *VPV = getOrAddLiveIn(CanonicalIVStartValue);
     auto *IV = getCanonicalIV();
     assert(all_of(IV->users(),
                   [](const VPUser *U) {
@@ -1091,7 +1091,7 @@ VPlan *VPlan::duplicate() {
   DenseMap<VPValue *, VPValue *> Old2NewVPValues;
   for (VPValue *OldLiveIn : VPLiveInsToFree) {
     Old2NewVPValues[OldLiveIn] =
-        NewPlan->getVPValueOrAddLiveIn(OldLiveIn->getLiveInIRValue());
+        NewPlan->getOrAddLiveIn(OldLiveIn->getLiveInIRValue());
   }
   Old2NewVPValues[&VectorTripCount] = &NewPlan->VectorTripCount;
   Old2NewVPValues[&VFxUF] = &NewPlan->VFxUF;
@@ -1102,7 +1102,7 @@ VPlan *VPlan::duplicate() {
   assert(TripCount && "trip count must be set");
   if (TripCount->isLiveIn())
     Old2NewVPValues[TripCount] =
-        NewPlan->getVPValueOrAddLiveIn(TripCount->getLiveInIRValue());
+        NewPlan->getOrAddLiveIn(TripCount->getLiveInIRValue());
   // else NewTripCount will be created and inserted into Old2NewVPValues when
   // TripCount is cloned. In any case NewPlan->TripCount is updated below.
 
@@ -1425,9 +1425,9 @@ VPValue *vputils::getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr,
     return Expanded;
   VPValue *Expanded = nullptr;
   if (auto *E = dyn_cast<SCEVConstant>(Expr))
-    Expanded = Plan.getVPValueOrAddLiveIn(E->getValue());
+    Expanded = Plan.getOrAddLiveIn(E->getValue());
   else if (auto *E = dyn_cast<SCEVUnknown>(Expr))
-    Expanded = Plan.getVPValueOrAddLiveIn(E->getValue());
+    Expanded = Plan.getOrAddLiveIn(E->getValue());
   else {
     Expanded = new VPExpandSCEVRecipe(Expr, SE);
     Plan.getPreheader()->appendRecipe(Expanded->getDefiningRecipe());
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index af6d0081bffebc..26162c25664e5b 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -2870,10 +2870,6 @@ class VPlan {
   /// definitions are VPValues that hold a pointer to their underlying IR.
   SmallVector<VPValue *, 16> VPLiveInsToFree;
 
-  /// Indicates whether it is safe use the Value2VPValue mapping or if the
-  /// mapping cannot be used any longer, because it is stale.
-  bool Value2VPValueEnabled = true;
-
   /// Values used outside the plan.
   MapVector<PHINode *, VPLiveOut *> LiveOuts;
 
@@ -2952,10 +2948,6 @@ class VPlan {
   /// Returns VF * UF of the vector loop region.
   VPValue &getVFxUF() { return VFxUF; }
 
-  /// Mark the plan to indicate that using Value2VPValue is not safe any
-  /// longer, because it may be stale.
-  void disableValue2VPValue() { Value2VPValueEnabled = false; }
-
   void addVF(ElementCount VF) { VFs.insert(VF); }
 
   void setVF(ElementCount VF) {
@@ -2985,25 +2977,22 @@ class VPlan {
   void setName(const Twine &newName) { Name = newName.str(); }
 
   void addVPValue(Value *V, VPValue *VPV) {
-    assert((Value2VPValueEnabled || VPV->isLiveIn()) &&
-           "Value2VPValue mapping may be out of date!");
-    assert(V && "Trying to add a null Value to VPlan");
-    assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
+    assert(VPV->isLiveIn());
     Value2VPValue[V] = VPV;
   }
 
   /// Returns the VPValue for \p V.
-  VPValue *getVPValue(Value *V) {
+  VPValue *getLiveIn(Value *V) {
     assert(V && "Trying to get the VPValue of a null Value");
     assert(Value2VPValue.count(V) && "Value does not exist in VPlan");
-    assert((Value2VPValueEnabled || Value2VPValue[V]->isLiveIn()) &&
-           "Value2VPValue mapping may be out of date!");
+    assert(Value2VPValue[V]->isLiveIn() &&
+           "Only live-ins should be in mapping");
     return Value2VPValue[V];
   }
 
   /// Gets the VPValue for \p V or adds a new live-in (if none exists yet) for
   /// \p V.
-  VPValue *getVPValueOrAddLiveIn(Value *V) {
+  VPValue *getOrAddLiveIn(Value *V) {
     assert(V && "Trying to get or add the VPValue of a null Value");
     if (!Value2VPValue.count(V)) {
       VPValue *VPV = new VPValue(V);
@@ -3011,7 +3000,7 @@ class VPlan {
       addVPValue(V, VPV);
     }
 
-    return getVPValue(V);
+    return getLiveIn(V);
   }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
@@ -3028,16 +3017,6 @@ class VPlan {
   LLVM_DUMP_METHOD void dump() const;
 #endif
 
-  /// Returns a range mapping the values the range \p Operands to their
-  /// corresponding VPValues.
-  iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
-  mapToVPValues(User::op_range Operands) {
-    std::function<VPValue *(Value *)> Fn = [this](Value *Op) {
-      return getVPValueOrAddLiveIn(Op);
-    };
-    return map_range(Operands, Fn);
-  }
-
   /// Returns the VPRegionBlock of the vector loop.
   VPRegionBlock *getVectorLoopRegion() {
     return cast<VPRegionBlock>(getEntry()->getSingleSuccessor());
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 6474a9697dce89..da246869625bb0 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -272,7 +272,7 @@ VPValue *PlainCFGBuilder::getOrCreateVPOperand(Value *IRVal) {
 
   // A and B: Create VPValue and add it to the pool of external definitions and
   // to the Value->VPValue map.
-  VPValue *NewVPVal = Plan.getVPValueOrAddLiveIn(IRVal);
+  VPValue *NewVPVal = Plan.getOrAddLiveIn(IRVal);
   IRDef2VPValue[IRVal] = NewVPVal;
   return NewVPVal;
 }
@@ -362,7 +362,7 @@ void PlainCFGBuilder::buildPlainCFG() {
   for (auto &I : *ThePreheaderBB) {
     if (I.getType()->isVoidTy())
       continue;
-    IRDef2VPValue[&I] = Plan.getVPValueOrAddLiveIn(&I);
+    IRDef2VPValue[&I] = Plan.getOrAddLiveIn(&I);
   }
 
   LoopBlocksRPO RPO(TheLoop);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index f6b564ad931ca9..93cbe7065db572 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -49,12 +49,11 @@ void VPlanTransforms::VPInstructionsToVPRecipes(
       if (auto *VPPhi = dyn_cast<VPWidenPHIRecipe>(&Ingredient)) {
         auto *Phi = cast<PHINode>(VPPhi->getUnderlyingValue());
         if (const auto *II = GetIntOrFpInductionDescriptor(Phi)) {
-          VPValue *Start = Plan->getVPValueOrAddLiveIn(II->getStartValue());
+          VPValue *Start = Plan->getOrAddLiveIn(II->getStartValue());
           VPValue *Step =
               vputils::getOrCreateVPValueForSCEVExpr(*Plan, II->getStep(), SE);
           NewRecipe = new VPWidenIntOrFpInductionRecipe(Phi, Start, Step, *II);
         } else {
-          Plan->addVPValue(Phi, VPPhi);
           continue;
         }
       } else {
@@ -625,9 +624,9 @@ void VPlanTransforms::optimizeForVFAndUF(VPlan &Plan, ElementCount BestVF,
     return;
 
   LLVMContext &Ctx = SE.getContext();
-  ...
[truncated]

github-actions · 2024-03-08T11:27:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ayalz · 2024-03-11T12:07:16Z

Instead of keeping a mapping of Inst->VPValues (of their corresponding recipes) in VPlan's Value2VPValue mapping, keep it in VPRecipeBuilder instead. After recently replacing the last user of this mapping after initial construction, this mapping is only needed for recipe construction (to map IR operands to VPValue operands).

By moving the mapping, VPlan's VPValue tracking can be simplified and limited only to live-ins. It also allows removing disableValue2VPValue and associated machinery & asserts.

Nice clean-up! Conceptually analogous to TransformState which keeps track of VPValue-to-Value(s) mapping during codegen only. Independent of this patch: should VPRecipeBuilder (or some other sub/class thereof) be confined to initial VPlan construction from LLVM-IR, excluding construction of recipes during VPlan-to-VPlan transformations?

ayalz · 2024-03-11T07:24:09Z

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h

@@ -172,6 +158,19 @@ class VPRecipeBuilder {
  /// Add the incoming values from the backedge to reduction & first-order
  /// recurrence cross-iteration phis.
  void fixHeaderPhis();
+
+  /// Returns a range mapping the values the range \p Operands to their


Suggested change

/// Returns a range mapping the values the range \p Operands to their

/// Returns a range mapping the values of the range \p Operands to their

(admittedly fixing an existing typo)

Done, thanks!

ayalz · 2024-03-11T07:26:39Z

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h

+  iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
+  mapToVPValues(User::op_range Operands, VPlan &Plan);
+
+  VPValue *getVPValue(Value *V, VPlan &Plan) {


OrAddLiveIn?

Updated, thanks!

ayalz · 2024-03-11T07:28:19Z

llvm/lib/Transforms/Vectorize/VPlan.h

-           "Value2VPValue mapping may be out of date!");
-    assert(V && "Trying to add a null Value to VPlan");
-    assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
+    assert(VPV->isLiveIn());


Error message missing.

added, thanks, also changed to addLiveIn

ayalz · 2024-03-11T07:32:42Z

llvm/lib/Transforms/Vectorize/VPlan.h

-  VPValue *getVPValueOrAddLiveIn(Value *V) {
+  VPValue *getOrAddLiveIn(Value *V) {


Why drop VPValue prefix? If desired, can be pushed separately?

I think it is more accurate (and compact), but I removed that change, to reduce the diff

Certainly more accurate (LiveIn is also a VPValue) and compact. Worth pushing independently.

ayalz · 2024-03-11T11:57:17Z

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h

@@ -117,13 +116,8 @@ class VPRecipeBuilder {
                                       VFRange &Range, VPBasicBlock *VPBB,
                                       VPlanPtr &Plan);

-  /// Set the recipe created for given ingredient. This operation is a no-op for
-  /// ingredients that were not marked using a nullptr entry in the map.
+  /// Set the recipe created for given ingredient.
  void setRecipe(Instruction *I, VPRecipeBase *R) {


Worth asserting to avoid resetting an already set recipe?

Added assert, thanks!

ayalz · 2024-03-11T11:59:52Z

llvm/lib/Transforms/Vectorize/VPlan.h

-    assert(V && "Trying to add a null Value to VPlan");
-    assert(!Value2VPValue.count(V) && "Value already exists in VPlan");


Worth retaining these asserts?

retained, thanks!

…value-mapping-for-non-live-outs

fhahn

Independent of this patch: should VPRecipeBuilder (or some other sub/class thereof) be confined to initial VPlan construction from LLVM-IR, excluding construction of recipes during VPlan-to-VPlan transformations?

Yes, VPRecipeBuilder ( not to be confused with VPBuilder) is only used for initial construction. We should split of the logic to create the initial VPlan to a separate helper, and only instantiate VPRecipeBuilder there

fhahn · 2024-03-11T21:33:12Z

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h

@@ -117,13 +116,8 @@ class VPRecipeBuilder {
                                       VFRange &Range, VPBasicBlock *VPBB,
                                       VPlanPtr &Plan);

-  /// Set the recipe created for given ingredient. This operation is a no-op for
-  /// ingredients that were not marked using a nullptr entry in the map.
+  /// Set the recipe created for given ingredient.
  void setRecipe(Instruction *I, VPRecipeBase *R) {


Added assert, thanks!

fhahn · 2024-03-11T21:33:59Z

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h

@@ -172,6 +158,19 @@ class VPRecipeBuilder {
  /// Add the incoming values from the backedge to reduction & first-order
  /// recurrence cross-iteration phis.
  void fixHeaderPhis();
+
+  /// Returns a range mapping the values the range \p Operands to their


Done, thanks!

fhahn · 2024-03-11T21:34:39Z

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h

+  iterator_range<mapped_iterator<Use *, std::function<VPValue *(Value *)>>>
+  mapToVPValues(User::op_range Operands, VPlan &Plan);
+
+  VPValue *getVPValue(Value *V, VPlan &Plan) {


Updated, thanks!

fhahn · 2024-03-11T21:36:39Z

llvm/lib/Transforms/Vectorize/VPlan.h

-           "Value2VPValue mapping may be out of date!");
-    assert(V && "Trying to add a null Value to VPlan");
-    assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
+    assert(VPV->isLiveIn());


added, thanks, also changed to addLiveIn

fhahn · 2024-03-11T21:37:28Z

llvm/lib/Transforms/Vectorize/VPlan.h

-    assert(V && "Trying to add a null Value to VPlan");
-    assert(!Value2VPValue.count(V) && "Value already exists in VPlan");


retained, thanks!

fhahn · 2024-03-11T21:41:01Z

llvm/lib/Transforms/Vectorize/VPlan.h

-  VPValue *getVPValueOrAddLiveIn(Value *V) {
+  VPValue *getOrAddLiveIn(Value *V) {


I think it is more accurate (and compact), but I removed that change, to reduce the diff

nikolaypanchenko · 2024-03-13T16:13:50Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -7938,7 +7950,7 @@ VPValue *VPRecipeBuilder::createEdgeMask(BasicBlock *Src, BasicBlock *Dst,
  if (OrigLoop->isLoopExiting(Src))
    return EdgeMaskCache[Edge] = SrcMask;

-  VPValue *EdgeMask = Plan.getVPValueOrAddLiveIn(BI->getCondition());
+  VPValue *EdgeMask = getVPValueOrAddLiveIn(BI->getCondition(), Plan);


Does it make more sense to move VPlan to VPRecipeBuilder ?

Forgot to mention, this has been split off & landed in 8578b6e, thanks!

@nikolaypanchenko

Instead of passing VPlan in a number of places, just store it directly in VPRecipeBuilder. A single instance is only used for a single VPlan. This simplifies the code and was suggested by @nikolaypanchenko in #84464.

…value-mapping-for-non-live-outs

ayalz · 2024-03-22T09:36:26Z

Independent of this patch: should VPRecipeBuilder (or some other sub/class thereof) be confined to initial VPlan construction from LLVM-IR, excluding construction of recipes during VPlan-to-VPlan transformations?

Yes, VPRecipeBuilder ( not to be confused with VPBuilder) is only used for initial construction. We should split of the logic to create the initial VPlan to a separate helper, and only instantiate VPRecipeBuilder there

Agreed. Would also be good to clarify the distinction between the two builders, possibly using more accurate names. May also be interesting to consider the analogous TransformState, used to maintain the temporary state during translation from VPlan to LLVM-IR.

ayalz

LGTM, a comment by @nikolaypanchenko awaits response.

ayalz · 2024-03-22T08:37:08Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

@@ -54,7 +54,6 @@ void VPlanTransforms::VPInstructionsToVPRecipes(
              vputils::getOrCreateVPValueForSCEVExpr(*Plan, II->getStep(), SE);
          NewRecipe = new VPWidenIntOrFpInductionRecipe(Phi, Start, Step, *II);
        } else {
-          Plan->addVPValue(Phi, VPPhi);


nit: now more appealing to swap and have an early continue.

Done thanks!

ayalz · 2024-03-22T09:41:32Z

llvm/lib/Transforms/Vectorize/VPlan.h

-  VPValue *getVPValueOrAddLiveIn(Value *V) {
+  VPValue *getOrAddLiveIn(Value *V) {


Certainly more accurate (LiveIn is also a VPValue) and compact. Worth pushing independently.

@nikolaypanchenko

Instead of passing VPlan in a number of places, just store it directly in VPRecipeBuilder. A single instance is only used for a single VPlan. This simplifies the code and was suggested by @nikolaypanchenko in llvm#84464.

…value-mapping-for-non-live-outs

github-actions · 2024-03-23T16:45:21Z

✅ With the latest revision this PR passed the Python code formatter.

fhahn requested review from ayalz and aniragil March 8, 2024 11:24

llvmbot added vectorizers llvm:transforms labels Mar 8, 2024

ayalz reviewed Mar 11, 2024

View reviewed changes

fhahn added 2 commits March 11, 2024 15:35

Merge remote-tracking branch 'origin/main' into vplan-remove-ir-to-vp…

c8d3d33

…value-mapping-for-non-live-outs

!fixup address comments

c509520

fhahn commented Mar 11, 2024

View reviewed changes

nikolaypanchenko reviewed Mar 13, 2024

View reviewed changes

fhahn added 2 commits March 18, 2024 19:30

Merge remote-tracking branch 'origin/main' into vplan-remove-ir-to-vp…

68add0e

…value-mapping-for-non-live-outs

!fixup after merge

ae8e890

ayalz approved these changes Mar 22, 2024

View reviewed changes

fhahn added 2 commits March 23, 2024 12:09

Merge remote-tracking branch 'origin/main' into vplan-remove-ir-to-vp…

44bd0c7

…value-mapping-for-non-live-outs

!fixup use early continue

bd52701

fhahn merged commit 39c8e87 into llvm:main Mar 23, 2024
4 checks passed

fhahn deleted the vplan-remove-ir-to-vpvalue-mapping-for-non-live-outs branch March 23, 2024 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VPlan] Move recording of Inst->VPValue to VPRecipeBuilder (NFCI). #84464

[VPlan] Move recording of Inst->VPValue to VPRecipeBuilder (NFCI). #84464

fhahn commented Mar 8, 2024

llvmbot commented Mar 8, 2024

github-actions bot commented Mar 8, 2024 •

edited

Loading

ayalz commented Mar 11, 2024

ayalz Mar 11, 2024

fhahn Mar 11, 2024

ayalz Mar 11, 2024

fhahn Mar 11, 2024

ayalz Mar 11, 2024

fhahn Mar 11, 2024

ayalz Mar 11, 2024

fhahn Mar 11, 2024

ayalz Mar 22, 2024

ayalz Mar 11, 2024

fhahn Mar 11, 2024

ayalz Mar 11, 2024

fhahn Mar 11, 2024

fhahn left a comment

fhahn Mar 11, 2024

fhahn Mar 11, 2024

fhahn Mar 11, 2024

fhahn Mar 11, 2024

fhahn Mar 11, 2024

fhahn Mar 11, 2024

nikolaypanchenko Mar 13, 2024

fhahn Mar 23, 2024

ayalz commented Mar 22, 2024

ayalz left a comment

ayalz Mar 22, 2024

fhahn Mar 23, 2024

ayalz Mar 22, 2024

github-actions bot commented Mar 23, 2024

	/// Returns a range mapping the values the range \p Operands to their
	/// Returns a range mapping the values of the range \p Operands to their

		VPValue getVPValueOrAddLiveIn(Value V) {
		VPValue getOrAddLiveIn(Value V) {

		assert(V && "Trying to add a null Value to VPlan");
		assert(!Value2VPValue.count(V) && "Value already exists in VPlan");

[VPlan] Move recording of Inst->VPValue to VPRecipeBuilder (NFCI). #84464

[VPlan] Move recording of Inst->VPValue to VPRecipeBuilder (NFCI). #84464

Conversation

fhahn commented Mar 8, 2024

llvmbot commented Mar 8, 2024

github-actions bot commented Mar 8, 2024 • edited Loading

ayalz commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayalz commented Mar 22, 2024

ayalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 8, 2024 •

edited

Loading